bpo-34822: Simplify AST for subscription. by serhiy-storchaka · Pull Request #9605 · python/cpython

serhiy-storchaka · 2018-09-27T17:11:18Z

Remove the slice type.
Make Slice a kind of the expr type instead of the slice type.
Replace ExtSlice(slices) with Tuple(slices, Load()).
Replace Index(value) with a value itself.

All non-terminal nodes in AST for expressions will be now of the expr type.

https://bugs.python.org/issue34822

* Remove the slice type. * Make Slice a kind of the expr type instead of the slice type. * Replace ExtSlice(slices) with Tuple(slices, Load()). * Replace Index(value) with a value itself. All non-terminal nodes in AST for expressions will be now of the expr type.

thautwarm · 2018-09-28T07:00:42Z

This enhancement is quite exciting, in fact, I've found the same problem at the midnight of yesterday, and it looks like a surprise to me that you made this PR now.

I'm working at an implementation of ast.parse through pure Python, and I found builtin ast.parse generates ast.ExtSlice instead of a composition of ast.Slice and ast.Tuple, which might produce inconsistency about ast structures.

However, for many projects depends on current AST structures, the changes might cause lots of incompatibilities. For instance, Hy project uses ExtSlice, and PonyORM, which uses Python ast to build SQL queries and related stuffs, and if they have used visit_Index or visit_ExtSlice, things would go bad.

AFAICS, we could add some facilities to ast module to guarantee compatibility to avoid the breakage of some 3-party packages that take advantage of python ast.

serhiy-storchaka · 2018-09-28T09:41:28Z

Hy project currently doesn't use ExtSlice and doesn't properly support extended slices (hylang/hy#1492). Seems merging this PR will make Hy automatically supporting extended slices.

PonyORM doesn't use Indox and ExtSlice.

Any project that just creates AST using functional style (Index(value=...) and ExtSlice(dims=...)) will continue to work (at least in few future releases). These names will be completely removed after they will grow out of use.

gvanrossum

LG, except for some wording nits.

(I say this even though merging this will complicate the PEG parser integration, because that currently can only be tested with Python 3.8.)

gvanrossum · 2020-03-09T17:53:47Z

Doc/library/ast.rst

+
+   Old classes :class:`ast.Index` and :class:`ast.ExtSlice` are still
+   available, but they will be removed in future Python releases.
+   In the meanwhile, instantiating them will return an instance of


I Googled this expression, and there seems to be a preference for either "In the meantime" or "Meanwhile" over "In the meantime". Up to you though, it's not wrong.

gvanrossum · 2020-03-09T18:02:58Z

Doc/library/ast.rst


-   Regular slicing (on the form x:y).
+   Regular slicing (on the form ``lower:upper`` or ``lower:upper:step``).
+   Can occur only in the *slice* field of :class:`Subscript`.


Maybe clarify that it can occur inside a Tuple that's directly inside a Subscript's slice field?

Done (not sure about correctness of my wording).

gvanrossum · 2020-03-09T18:13:51Z

Lib/ast.py

 }

+class Index(AST):
+    def __new__(cls, value, *args, **kwargs):


Are there ever any additional positional args? (I presume kwargs could be lineno etc.)

gvanrossum · 2020-03-09T18:15:22Z

Lib/ast.py

+        return value
+
+class ExtSlice(AST):
+    def __new__(cls, dims=(), *args, **kwargs):


Why not make dims a mandatory argument, like value for Index?

Index without value cannot work. This change breaks the following code

node = Index() node.value = Constant(1)

but we cannot keep it working. We should have something to return from Index().

On other side, ExtSlice with empty dims is invalid, and the following legal code

node = ExtSlice() node.dims.append(Ellipsis())

will be broken because Tuple has the elts filed instead of dims. We can add an alias dims in Tuple to make the above example working. Should we? If not, than making dims a required parameter will not break anything that is not broken.

P.S. There is a test (test_field_attr_existence) which tests that all node classes are callable without arguments (Index is an exception now). Should we make ExtSlice an exception too?

gvanrossum · 2020-03-09T18:16:40Z

Lib/ast.py

-            if (isinstance(node.slice, Index)
-                    and isinstance(node.slice.value, Tuple)
-                    and node.slice.value.elts):
-                if len(node.slice.value.elts) == 1:
-                    elt = node.slice.value.elts[0]
+            if isinstance(node.slice, Tuple) and node.slice.elts:
+                if len(node.slice.elts) == 1:
+                    elt = node.slice.elts[0]
                    self.traverse(elt)
                    self.write(",")
                else:
-                    self.interleave(lambda: self.write(", "), self.traverse, node.slice.value.elts)
+                    self.interleave(lambda: self.write(", "), self.traverse, node.slice.elts)


I'm guessing you'll get a merge conflict here if GH-17892 is merged first (which I think it should be).

gvanrossum · 2020-03-09T19:36:18Z

Misc/NEWS.d/next/Library/2018-09-27-19-31-47.bpo-34822.EztBhL.rst

+Simplified AST for subscription. Simple indices will be represented by their
+value, extended slices will be represented as tuples.  :mod:`ast` classes
+``Index`` and ``ExtSlice`` are considered deprecated and will be removed in
+future Python versions. In meanwhile, ``Index(value)`` will return a ``value``
+itself, ``ExtSlice(slices)`` will return ``Tuple(slices, Load())``.


Maybe replace "will be" with "are now"? Also change "will return" into "now returns". (But "will be removed" must stay, since it is truly in the future.)

Doc/whatsnew/3.9.rst

gvanrossum

LGTM now. Go ahead!

isidentical

Looks great.

serhiy-storchaka · 2020-03-10T16:53:19Z

Thank you for your review @gvanrossum!

gvanrossum · 2020-03-10T17:13:58Z

You're welcome. Thanks for caring so much about Python.

The structure of the AST for subscripts was changed in python/cpython#9605 Fixes #8627

The structure of the AST for subscripts was changed in python/cpython#9605. This also fixes some systemic issues with the tests under Python 3.9, but not all. Fixes #8627

* Remove the slice type. * Make Slice a kind of the expr type instead of the slice type. * Replace ExtSlice(slices) with Tuple(slices, Load()). * Replace Index(value) with a value itself. All non-terminal nodes in AST for expressions are now of the expr type.

serhiy-storchaka added the type-feature A feature request or enhancement label Sep 27, 2018

the-knights-who-say-ni added the CLA signed label Sep 27, 2018

bedevere-bot added the awaiting merge label Sep 27, 2018

thautwarm mentioned this pull request Sep 28, 2018

Properly Generate ExtSlice hylang/hy#1492

Closed

auvipy approved these changes Oct 1, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Oct 1, 2018

Merge branch 'master' into ast-slice

add0184

auvipy approved these changes Nov 3, 2018

View reviewed changes

serhiy-storchaka added 6 commits December 3, 2018 20:35

Merge branch 'master' into ast-slice

d15dfec

Merge branch 'master' into ast-slice

9f415fa

Fix new tests.

4a08f6d

Merge branch 'master' into ast-slice

42fef82

Merge branch 'master' into ast-slice

fe6989b

Merge branch 'master' into ast-slice

dbc3309

serhiy-storchaka mentioned this pull request Feb 15, 2020

bpo-32892: Update the documentation for handling constants in AST. #18514

Merged

serhiy-storchaka added 5 commits February 16, 2020 23:08

Fix merge.

665476a

Merge branch 'master' into ast-slice

028e0fd

Merge branch 'master' into ast-slice

df65f31

Merge branch 'master' into ast-slice

e6e9ec7

Merge branch 'master' into ast-slice

aaa13c3

gvanrossum mentioned this pull request Mar 9, 2020

Changes to AST coming in 3.9 we-like-parsers/pegen_experiments#228

Closed

gvanrossum approved these changes Mar 9, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 9, 2020

serhiy-storchaka added 2 commits March 9, 2020 21:52

Merge branch 'master' into ast-slice

374bf22

Adddress review comments, fix docs, add the dims getter in Tuple.

f2226f3

serhiy-storchaka added 2 commits March 9, 2020 23:18

Merge branch 'master' into ast-slice

338e13d

Merge branch 'master' into ast-slice

6cd32fa

gvanrossum reviewed Mar 9, 2020

View reviewed changes

Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved

serhiy-storchaka added 2 commits March 10, 2020 09:04

Remove trailing spaces.

a338ada

Merge branch 'master' into ast-slice

6e721c9

gvanrossum approved these changes Mar 10, 2020

View reviewed changes

isidentical approved these changes Mar 10, 2020

View reviewed changes

serhiy-storchaka merged commit 13d52c2 into python:master Mar 10, 2020

bedevere-bot removed the awaiting merge label Mar 10, 2020

serhiy-storchaka deleted the ast-slice branch March 10, 2020 16:52

tacaswell mentioned this pull request Mar 17, 2020

pandas does not import with cpython master pandas-dev/pandas#32766

Closed

gvanrossum mentioned this pull request Mar 19, 2020

How to go about including pegen code in CPython? we-like-parsers/cpython#7

Closed

lysnikolaou mentioned this pull request Mar 26, 2020

Remove all unnecessary code from Tools/peg_generator; only keep the generator itself we-like-parsers/cpython#23

Merged

msullivan mentioned this pull request Apr 3, 2020

Update fastparse for 3.9 AST changes python/mypy#8627

Closed

gvanrossum mentioned this pull request Apr 16, 2020

Add func_type_input to possible start rules we-like-parsers/cpython#36

Closed

gvanrossum added a commit to python/mypy that referenced this pull request Apr 23, 2020

Fix parser when using Python 3.9

43cb4eb

The structure of the AST for subscripts was changed in python/cpython#9605 Fixes #8627

gvanrossum mentioned this pull request Apr 23, 2020

Fix parser when using Python 3.9 python/mypy#8716

Merged

Uh oh!

Conversation

serhiy-storchaka commented Sep 27, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thautwarm commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Sep 28, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

isidentical left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Mar 10, 2020

Uh oh!

gvanrossum commented Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

serhiy-storchaka commented Sep 27, 2018 •

edited by bedevere-bot

Loading

thautwarm commented Sep 28, 2018 •

edited

Loading